Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Member
Author
|
run buildall |
1 similar comment
Member
Author
|
run buildall |
Member
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Static review at exact head 3606a1a2af1b2a3632f0c6d57e89e44959f55205. I found six distinct issues: three can return wrong results or query the wrong index (selector collision, outer-join SEARCH(NULL), and analyzer-selected EXACT), one newly rejects a valid passthrough alias, one rejects equivalent analyzer spellings, and one evaluates the same MATCH virtual expression twice.
Checkpoint conclusions: the change is focused on the necessary analyzer, rewrite, expression, and translation stages, and the changed tests broadly cover direct/VARIANT OLAP joins, MOW behavior, TERM/PHRASE analyzers, and typed VARIANT TopN. They do not cover the six inline triggers. No new shared-state concurrency, lifecycle/static initialization, configuration, persistence/EditLog, transaction/write, or storage-format mechanism is introduced. Existing field-binding/index-properties transport remains positionally aligned for ordinary and VARIANT slots, but it cannot preserve explicit-selector intent for EXACT as implemented. Join types, ExprIds/nullability, predicate movement, top-down reentry, direct/residual paths, options fields, generated outputs, ordering, and negative-test form were checked; the duplicate virtual MATCH is the only additional performance issue. Existing diagnostics are otherwise sufficient.
This was a static-only review as required: I did not run builds or tests. The author reports validation on an older development baseline, and current exact-head CI is separate from that claim. There was no additional user focus and no existing inline review thread to duplicate.
| while (analyzerSeparator > 0 && bindingName.charAt(analyzerSeparator - 1) == '\\') { | ||
| analyzerSeparator = bindingName.lastIndexOf('@', analyzerSeparator - 1); | ||
| } | ||
| if (analyzerSeparator >= 0 && findSlotByName(bindingName, scan) == null) { |
Contributor
There was a problem hiding this comment.
[P1] Preserve analyzer-selector syntax before schema lookup. For an unquoted name@english the parser leaves @ unescaped, but this branch treats it as a selector only when a slot named name@english does not exist. If a table has both indexed name and a literal name@english column, the query silently binds the literal column and its index instead of applying the english analyzer to name, so it can return different rows. Please carry quoted/unquoted provenance through parsing and split every unquoted selector independently of slot collisions, with a collision regression.
| } | ||
| String fieldKey = normalizedFieldName.toLowerCase(Locale.ROOT); | ||
| if (fieldAnalyzers.containsKey(fieldKey) | ||
| && !Objects.equals(fieldAnalyzers.get(fieldKey), binding.getAnalyzerName())) { |
Contributor
There was a problem hiding this comment.
[P2] Compare analyzer identities with the same normalization used for index lookup. isAnalyzerMatched accepts analyzer names case-insensitively, so both name@CRM_DOC_TEXT and name@crm_doc_text resolve to the same index, but this Objects.equals check then rejects them as two analyzers for one field. Normalize with Locale.ROOT (or compare case-insensitively) and cover mixed-case spellings in one DSL.
| originalFieldName, search.getDslString())); | ||
| } | ||
| checkInvertedIndexExists(scan.getTable(), slot.getName(), search.getDslString(), false); | ||
| checkInvertedIndexExists(tableForSlot(slot, scan), slot.getName(), search.getDslString(), false); |
Contributor
There was a problem hiding this comment.
[P1] Check the physical original column rather than the visible alias. For a passthrough such as (SELECT content AS body FROM t) s, findSlotByName returns body and that slot retains originalTable=t/originalColumn=content, but this call asks t for a column named body and falsely reports that no index exists. The VARIANT parent path has the same alias issue. Please use the slot's original column (plus subpath where applicable) for index validation while retaining the alias only for DSL binding, and add renamed-output regressions.
| for (Expression child : children) { | ||
| if (!(child instanceof SlotReference || child instanceof ElementAt)) { | ||
| if (!(child instanceof SlotReference || child instanceof ElementAt | ||
| || child instanceof NullLiteral)) { |
Contributor
There was a problem hiding this comment.
[P1] Do not let inference-only NULL children persist into executable SEARCH plans. For a LEFT JOIN b ON FALSE with search('content:john') IS NULL on b, join elimination produces NULL AS content and filter-through-project substitutes it here, yielding Search(NULL) directly over a's scan. The materializer skips it, the final Filter-to-scan check admits it, and BE's no-iterator path produces empty data and null bitmaps, so SEARCH is false/non-null and the preserved rows are wrongly rejected. Please keep symbolic NULL replacement non-persistent or reject non-slot/subcolumn children before translation, and cover false outer-join padding.
| return null; | ||
| } | ||
| List<NamedExpression> projects = new ArrayList<>(project.getProjects()); | ||
| projects.add(result.second); |
Contributor
There was a problem hiding this comment.
[P2] Record or reuse the materialization before leaving this project. If the same MATCH appears in this child projection and a preserved-side outer-join ON condition, pushDownJoin first reaches this path and appends one virtual slot; top-down traversal then reaches the rebuilt project, where the direct Project-to-scan rule allocates a second alias because it does not consult the scan's existing virtual columns. Both ExprIds stay referenced and the segment iterator evaluates/materializes the predicate twice. Please centralize the reuse check and assert this plan has one virtual MATCH column.
| Column column = slot.getOriginalColumn().orElse(null); | ||
| if (column != null) { | ||
| invertedIndex = olapTbl.getInvertedIndex(column, slot.getSubPath()); | ||
| invertedIndex = olapTbl.getInvertedIndex(column, slot.getSubPath(), analyzer); |
Contributor
There was a problem hiding this comment.
[P1] Ensure this selected analyzer also constrains BE reader choice for EXACT. FE resolves the requested index here and sends its properties, but FieldReaderResolver derives analyzer_key only when the query type is not EQUAL_QUERY; SEARCH maps EXACT to EQUAL_QUERY. Two custom standard/keyword analyzers are both FULLTEXT readers, so the empty-key selector can pick the lower index ID instead of the requested keyword analyzer and return different rows. Please honor an explicit analyzer for every clause type (then apply type preference within that analyzer) and add a two-analyzer EXACT regression.
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 16789 ms |
Contributor
TPC-DS: Total hot run time: 81601 ms |
Contributor
ClickBench: Total hot run time: 14.85 s |
…e an outer join's NULL side ### What problem does this PR solve? Issue Number: None Related PR: apache#67932 Problem Summary: PushDownProject pushes every PreferPushDownProject expression (MATCH, element_at, ...) used by a filter or project above a join into the child that outputs its slots. When that child is the NULL-extended side of an outer join, the join pads the pushed value with NULL, but the same expression evaluated above the join can be non-NULL for NULL input. Reproduce with SELECT b.k1 FROM b LEFT JOIN a ON b.k1 = a.k1 WHERE nvl(a.content, 'hello') MATCH_ANY 'hello' OR b.k1 = 100 Rows of b without a join partner satisfy the predicate (nvl(NULL, 'hello') matches), yet they were dropped because the MATCH was computed inside a and then padded with NULL. The same happened to such an expression in the SELECT list, which returned NULL instead of TRUE. The fix adds ExpressionUtils.isNullPropagating, built on the existing replace-slots-with-NULL-and-fold inference (matchesWhenSlotsAreNull, generalized from a single slot), and PushDownProject only pushes an expression into a NULL-extended join child when it is NULL for NULL input. The scan virtual column rule of the related PR uses the same helper. SearchExpression is excluded from BE constant folding like Search, because that inference replaces its slots with NULL literals. ### Release note Fix wrong results when a MATCH (or another pushed-down expression) whose operand turns NULL into a value, such as nvl(col, 'x') MATCH_ANY 'x', is evaluated over the NULL-extended side of an outer join. ### Check List (For Author) - Test: Unit Test (PushDownProjectTest, ExpressionUtilsTest) and Regression test (search/test_crm_search_join_document: nullside_nonstrict_match_where, nullside_nonstrict_match_select) - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…d execution checks ### What problem does this PR solve? Issue Number: None Related PR: apache#67932 Problem Summary: Review of SEARCH in joins found correctness and maintainability problems that share a few causes: several places decided the same thing differently, and checks looked at plan shapes instead of what BE executes. Correctness 1. NULL padding could bypass the outer join guard. `LEFT JOIN a ON false`, an outer join converted to an anti join, and alias inlining of `NULL AS col` replace a SEARCH field with a NULL literal; the expression then sat on the preserved scan where BE found no indexed field and evaluated it as FALSE, so `NOT search(...) OR b.k1 = 8` returned every row. CheckAfterRewrite now verifies, in one place and also for scan virtual columns, that every SearchExpression is evaluated by a scan and still binds only index fields. 2. Predicate inference copied a SEARCH to an equal column. With `a JOIN n ON a.content = n.name WHERE search('a.content:hello') OR a.content = 'zzz'`, InferPredicateByReplace produced `search[n.name] OR n.name = 'zzz'` for n, whose column has no inverted index, and the query returned no rows instead of four. The positional inference of INTERSECT/EXCEPT in InferPredicates rebinds a pulled-up SEARCH the same way (an INTERSECT with a SEARCH branch returned no rows). A SEARCH is bound to the indexes of its own columns, so it is neither an input of equality inference nor cloned into a sibling branch, like volatile expressions. 3. `field@analyzer` was read as a column named `field@analyzer` whenever such a column existed. The selector is now purely syntactic and owned by SearchDslParser.splitAnalyzerSelector: the last unescaped `@` after a non-empty path segment selects the analyzer; `\@`, an `@` inside a quoted segment and an `@` that starts a segment (`v.@timestamp`) belong to the name. 4. Index validation and the field names sent to BE used the slot's output name. After `SELECT content AS body` or `v AS props` a valid SEARCH failed with "Column not found", or validated another column that happened to carry the alias name. Names are resolved by their visible name; validation, NESTED paths and BE field names use the slot's original table and column. Generality and maintainability 5. One materialization path. Project over a scan only unwrapped `Alias(match)` while residual filters and joins collected searches recursively, so `CASE WHEN col MATCH ... END` behaved differently by plan shape. The rule (renamed PushDownIndexSearchAsVirtualColumn; the rule type keeps its name for disable_nereids_rules) has a single collect, materialize, replace path, no longer appends a reused virtual column twice, and refuses to inline a volatile alias producer. 6. Responsibilities are explicit: CheckSearchUsage checks placement on the analyzed plan (a necessary condition only), materialize() alone decides where an index search may be computed, CheckAfterRewrite verifies the final plan. The qualifier-set "one table" check is gone; a SEARCH over two relations can never reach one scan and is rejected by the final check with the constraints spelled out. 7. Field names resolve like SQL column references by reusing ExpressionAnalyzer.bindSlotByScope, so `alias.field` selects one side of a self join before `column.subcolumn` is tried, and an ambiguous name is an error. 8. "One analyzer per field" compares the inverted indexes selected by OlapTable.getInvertedIndex, the lookup the translator sends to BE, instead of analyzer strings, so `name@Exact` and `name@exact` agree. Variant subcolumn paths keep their case when the plan fields are normalized. 9. Messages and comments no longer say "single-table scans"; the dead Rewriter registration of RewriteSearchToSlots is removed because binding happens in the Analyzer. Tests The pipeline randomizes the VARIANT defaults. With a small default_variant_max_subcolumns_count a subcolumn is stored in the sparse column without an inverted index, which made test_crm_search_join_document fail with "match_all not support execute_match" and test_crm_search_analyzers return an empty result (both reproduced by setting the variable by hand). The three CRM suites now pin those defaults like the other search suites. ### Release note In a SEARCH field reference an unquoted `@` always selects an analyzer; quote the segment or write `\@` for a literal `@` that follows a field name. SEARCH fields may be qualified with a table alias (`search('a.title:x')`). ### Check List (For Author) - Test: Unit Test (SearchJoinDocumentTest, RewriteSearchToSlotsTest, PushDownIndexSearchAsVirtualColumnTest, SearchDslParserTest, SearchExpressionTest, CheckSearchUsageTest and the FE tests that reference the changed rules) and Regression test (search/, inverted_index_p0/test_match_projection_virtual_column) - Behavior changed: Yes (see release note; error messages for unsupported SEARCH placement are more specific) - Does this need documentation: Yes (analyzer selector and table alias syntax of SEARCH fields) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 27992 ms |
Contributor
TPC-DS: Total hot run time: 153333 ms |
Contributor
TPC-H: Total hot run time: 28283 ms |
Contributor
TPC-DS: Total hot run time: 154033 ms |
Contributor
ClickBench: Total hot run time: 24.28 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
SEARCH in WHERE is rejected when its input contains an OLAP join. Residual predicates such as
(MATCH AND EXISTS (...)) OR joined_column = ...can also reach execution without an inverted-index evaluation path. Filtering the scan by MATCH alone would incorrectly remove rows selected by the other OR branch.This PR binds SEARCH field dependencies before pruning and predicate movement, and extends the existing scan virtual-column rule to materialize MATCH/SEARCH booleans used by projections, residual filters, and join conditions. The original SQL boolean expression and join multiplicity are preserved. This includes WHERE predicates moved into INNER JOIN conditions by the optimizer.
It also supports per-field analyzer selection, for example
search('name@exact:"John Smith" AND title@text:software'), including selectors in thefieldsoption. Selected index properties use the existing FE/BE interface. Quoted literal@field names remain supported.Each SEARCH expression still references one table instance; separate SEARCH expressions can be combined across tables using SQL AND/OR. SEARCH across an outer join's null-generating side remains conservatively gated. Explicit SEARCH projections/ON clauses, tuple IN subqueries, analyzer-IN, and multiple analyzers for the same field within one SEARCH are outside this change.
No BE code, storage format, Thrift, or new plan-node changes are included. Typed VARIANT TopN uses existing lazy materialization; sorting an untyped VARIANT value still requires an explicit cast.
Release note
Support SEARCH predicates in OLAP join queries and per-field analyzer selection, and evaluate residual MATCH/SEARCH expressions through indexed scan virtual columns.
Check List (For Author)
Validation on the original development baseline
16ab0566e9796d0498e6d2b3221e2a59d5e94ef7with these changes:test_crm_search_join_document,test_crm_search_analyzers,test_crm_search_variant_topn,test_search_usage_restrictions,test_search_null_semantics,test_search_variant_subcolumn_analyzer, andtest_match_projection_virtual_column.match_any not support execute_matchfor the OR/EXISTS control query; enabling it returned the expected five rows.For this PR, only the two feature/test commits were cherry-picked onto master
df36e174b99557a004bc3ad57faa019da4a7d91f. Range comparison shows unchanged test changes and only surrounding Analyzer context differences. Tests have not been rerun on this rebased master head; the draft records that validation boundary explicitly. No C++ files changed, so clang-format is not applicable.Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)